Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose force_quotes on Windows. #77728

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Expose force_quotes on Windows. #77728

merged 1 commit into from
Feb 18, 2021

Conversation

lygstate
Copy link
Contributor

@lygstate lygstate commented Oct 8, 2020

On Windows, the arg quotes and not quotes have different effect
for the program it called, if the program called are msys2/cygwin program.
Refer to
msys2/MSYS2-packages#2176

This also solve the issues comes from

https://internals.rust-lang.org/t/std-process-on-windows-is-escaping-raw-literals-which-causes-problems-with-chaining-commands/8163

Tracking issue:
#82227

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020
@camelid camelid added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 8, 2020
@lygstate lygstate force-pushed the master branch 8 times, most recently from 92c8614 to 88e3da1 Compare October 9, 2020 13:24
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2020
@Dylan-DPC-zz
Copy link

not sure who is a good candidate to review this

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

r? @Amanieu perhaps, though not sure. May be worth pinging the windows group.

@jyn514
Copy link
Member

jyn514 commented Nov 24, 2020

@retep998 what do you think?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2020
@sivadeilra
Copy link

I'll take a look.

Copy link

@sivadeilra sivadeilra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, the PR makes sense. There are two things that concern me, though:

  1. Since the feature is exposed by defining a new function on an existing trait, this could be a breaking change. I know it's an obscure case, but still, the Rust Project aims to provide an objective definition of non-breaking changes.
  2. This feature works on an entire command-line, rather than individual args. So this solves your current need, but it's easy to imagine a case in the future where you wanted to pass an arg string like "foo*.c" bar*, which would not be possible with this PR.

Perhaps a better approach would be to allow the caller to provide a verbatim String that provides exactly the arg that is passed to CreateProcess. That way, callers who know they're on Windows and know that they need control over argument parsing can provide exactly the string that they want.

/// In msvcrt based program, the commandline with and without quotes have
/// the same effect.
/// Refer to https://msdn.microsoft.com/en-us/library/17w5ykft.aspx
#[unstable(feature = "windows_process_extensions_force_quotes", issue = "77728")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unlikely but possible that this could be a breaking change. If someone implemented CommandExt in a separate crate, then adding this new associated function will break that implementation. I know, it's really unlikely, but still.

This could be prevented by either 1) moving force_quotes to a new trait, or 2) providing a default implementation of force_quotes which did nothing, but returned self. I'm not sure that's possible, though, given how the function signature is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does trait are possible to implement defaut implementation?

Copy link
Member

@camelid camelid Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lygstate Traits can have default implementations for methods, if that's what you mean.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lygstate For example, you can do this:

pub trait Foo {
  fn foo(&self);   // <-- has no default

  fn bar(&self) {
    println!("I am the default behavior");
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's impossible to implement default implementation

    #[unstable(feature = "windows_process_extensions_force_quotes", issue = "77728")]
    fn force_quotes(&mut self, enabled: bool) -> &mut process::Command {
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if there's no reasonable default implementation that satisfies the expected behavior, then it doesn't make sense to have a default implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However in this case you can probably just return self as @sivadeilra suggested. Having an empty body is definitely not going to work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camelid I am sorry for that, return self are also not working and I've tried.
It's trait and not inherit from process::Command, so return self not working

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 11, 2021
Seal the CommandExt, OsStrExt and OsStringExt traits

A crater run (rust-lang#81213 (comment)) has shown that this does not break any existing code.

This also unblocks rust-lang#77728.

Based on rust-lang#81213.

r? ``@m-ou-se``
cc ``@lygstate``
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that #81975 is merged, we can move forward with this.

library/std/src/sys/windows/ext/process.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se removed I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 17, 2021
@Amanieu
Copy link
Member

Amanieu commented Feb 17, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2021

📌 Commit c38648afe61e6f84d7e3859a53c67e84bcb505fa has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2021
@Amanieu
Copy link
Member

Amanieu commented Feb 17, 2021

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 17, 2021
@Amanieu
Copy link
Member

Amanieu commented Feb 17, 2021

You need to create a tracking issue for this feature and update the unstable attribute to point to it.

@lygstate
Copy link
Contributor Author

You need to create a tracking issue for this feature and update the unstable attribute to point to it.

Hi, any example?

@m-ou-se
Copy link
Member

m-ou-se commented Feb 17, 2021

You can use this template for the tracking issue.

@lygstate lygstate changed the title Expose force_quotes, Expose force_quotes on Windows. Feb 17, 2021
Quotes the arg and not quotes the arg have different effect on Windows when the program called
are msys2/cygwin program.
Refer to msys2/MSYS2-packages#2176

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
@Amanieu
Copy link
Member

Amanieu commented Feb 17, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2021

📌 Commit fa23ddf has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77728 (Expose force_quotes on Windows.)
 - rust-lang#80572 (Add a `Result::into_ok_or_err` method to extract a `T` from `Result<T, T>`)
 - rust-lang#81860 (Fix SourceMap::start_point)
 - rust-lang#81869 (Simplify pattern grammar, improve or-pattern diagnostics)
 - rust-lang#81898 (Fix debug information for function arguments of type &str or slice.)
 - rust-lang#81972 (Placeholder lifetime error cleanup)
 - rust-lang#82007 (Implement reborrow for closure captures)
 - rust-lang#82021 (Spell out nested Self type in lint message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit db59950 into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.